Skip to content

url, test: add inspect to TupleOrigin#10039

Closed
captainsafia wants to merge 4 commits intonodejs:masterfrom
captainsafia:add-inspect-to-tuple-origin
Closed

url, test: add inspect to TupleOrigin#10039
captainsafia wants to merge 4 commits intonodejs:masterfrom
captainsafia:add-inspect-to-tuple-origin

Conversation

@captainsafia
Copy link
Contributor

@captainsafia captainsafia commented Dec 1, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

url, test

Description of change

This PR adds code and tests for an inspect function on the TupleOrigin object.

@captainsafia
url: add inspect function to TupleOrigin
c6b324e 
This adds a simple inspect function the the TupleOrigin class.
@captainsafia
test: add test for inspect in TupleOrigin
b930653 
This adds tests for the newly added inspect function in the TupleOrigin
class.
@nodejs-github-bot nodejs-github-bot added the url Issues and PRs related to the legacy built-in url module. label Dec 1, 2016
@captainsafia captainsafia changed the title url, test: add inspect to tuple origin url, test: add inspect to TupleOrigin Dec 1, 2016
@imyller imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016
@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 2, 2016
@Trott
Copy link
Member

Trott commented Dec 2, 2016

/cc @jasnell @addaleax

addaleax
addaleax approved these changes Dec 2, 2016
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a suggestion, thanks!

lib/internal/url.js Outdated
}

inspect() {
return `TupleOrigin{
Copy link
Member

@addaleax addaleax Dec 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have spaces before the { in most cases here?

cjihrig
cjihrig approved these changes Dec 2, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with @addaleax's nit addressed and a CI run.

@jasnell
Copy link
Member

jasnell commented Dec 2, 2016 via email

@captainsafia
Copy link
Contributor Author

captainsafia commented Dec 2, 2016

Nit addressed.

@gibfahn
Copy link
Member

gibfahn commented Dec 2, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/5122/

@addaleax
Copy link
Member

addaleax commented Dec 2, 2016

@captainsafia Looks like you need to update the test files too? :)

@addaleax
Copy link
Member

addaleax commented Dec 2, 2016

Thanks for being so quick to update! Here’s a new CI: https://ci.nodejs.org/job/node-test-commit/6348/

@captainsafia
Copy link
Contributor Author

captainsafia commented Dec 2, 2016

@addaleax: Whoops. Excuse my merge-eager fingers. Fixed now. 😊

@addaleax
Copy link
Member

addaleax commented Dec 5, 2016

Landed in 8831732, thanks for the contribution!

@addaleax addaleax closed this Dec 5, 2016
addaleax pushed a commit that referenced this pull request Dec 5, 2016
@captainsafia @addaleax
url: add inspect function to TupleOrigin
8831732 
This adds a simple inspect function the the TupleOrigin class.
This adds tests for the newly added inspect function in the TupleOrigin
class.

PR-URL: #10039
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@jasnell
Copy link
Member

jasnell commented Dec 5, 2016

Thank you for this @captainsafia ... this one has been on my list of todo's for remaining items on the new URL implementation and I really appreciate the contribution!

@captainsafia captainsafia deleted the add-inspect-to-tuple-origin branch December 5, 2016 07:09
jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
@captainsafia
url: add inspect function to TupleOrigin
a760575 
This adds a simple inspect function the the TupleOrigin class.
This adds tests for the newly added inspect function in the TupleOrigin
class.

PR-URL: nodejs#10039
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 15, 2016
@captainsafia @MylesBorins
url: add inspect function to TupleOrigin
24482d0 
This adds a simple inspect function the the TupleOrigin class.
This adds tests for the newly added inspect function in the TupleOrigin
class.

PR-URL: #10039
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@italoacasas italoacasas mentioned this pull request Dec 15, 2016
Merged
italoacasas pushed a commit that referenced this pull request Dec 15, 2016
2016-12-20, Version 7.3.0 (Current)
617dbf7 
Notable changes

SEMVER-MINOR
- url:
    - add inspect function to TupleOrigin (Safia Abdalla) #10039
- crypto:
    - allow adding extra certs to well-known CAs (Sam Roberts) #9139

SEMVER-PATCH
- buffer:
    - fix single-character string filling (Anna Henningsen) #9837
    - handle UCS2 .fill() properly on BE (Anna Henningsen) #9837
- url:
    - including base argument in originFor (joyeecheung) #10021
    - improve URLSearchParams spec compliance (Timothy Gu) #9484
- http:
    - remove stale timeout listeners (Karl Böhlmark) #9440
- build:
    - fix node_g target (Daniel Bevenius) #10153
- fs:
    - remove unused argument from copyObject() (Ethan Arrowood) #10041
- timers:
    - fix handling of cleared immediates (hveldstra) #9759

PR-URL: #10277
italoacasas pushed a commit that referenced this pull request Dec 17, 2016
2016-12-20, Version 7.3.0 (Current)
0afa836 
Notable changes:
* **crypto**:
  - Allow adding extra certificates to well-known CAs. (Sam Roberts)
    [#9139](#9139)
* **buffer**:
  - Fix single-character string filling. (Anna Henningsen)
    [#9837](#9837)
  - Handle UCS2 `.fill()` properly on BE. (Anna Henningsen)
    [#9837](#9837)
* **url**:
  - Add inspect function to TupleOrigin. (Safia Abdalla)
    [#10039](#10039)
  - Including base argument in originFor. (joyeecheung)
    [#10021](#10021)
  - Improve URLSearchParams spec compliance. (Timothy Gu)
    [#9484](#9484)
* **http**:
  - Remove stale timeout listeners. (Karl Böhlmark)
    [#9440](#9440)
* **build**:
  - Fix node_g target. (Daniel Bevenius)
    [#10153](#10153)
* **fs**:
  - Remove unused argument from copyObject(). (EthanArrowood)
    [#10041](#10041)
* **timers**:
  - Fix handling of cleared immediates. (hveldstra)
    [#9759](#9759)
* **src**:
  - Add wrapper for process.emitWarning(). (SamRoberts)
    [#9139](#9139)
  - Fix string format mistake for 32 bit node.(Alex Newman)
    [#10082](#10082)
@joyeecheung joyeecheung added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@gibfahn gibfahn gibfahn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. semver-minor PRs that contain new features and should be released in the next minor version. url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@captainsafia @Trott @jasnell @gibfahn @addaleax @cjihrig @imyller @joyeecheung @nodejs-github-bot